Fix `–-toolchain` value shadowed by local executables #7483

MaxDesiatov · 2024-04-23T11:35:20Z

Behavior of --toolchain regressed in https://github.com/apple/swift-package-manager/pull/7296/files by preferring the first match in toolset root paths: when swiftc is available in the same directory next to the SwiftPM binary being invoked, that became preferred with --toolchain value ignored.

This is now fixed by slightly tweaking where custom toolchain gets added - it's prepended.

A newly added test verifies that the behavior is fixed. For that we had to inject FileSystem and EnvironmentVariables in a lot more places, with an added benefit of making our tests more consistent and isolated from an arbitrary build environment.

Resolves: rdar://126095653

We should prevent possible regressions in how this argument is handled.

MaxDesiatov · 2024-04-23T11:35:26Z

@swift-ci test

Since #7483 this type is implemented in SwiftPM.

MaxDesiatov · 2024-04-23T11:39:14Z

@swift-ci test

MaxDesiatov · 2024-04-23T12:03:46Z

@swift-ci test

MaxDesiatov · 2024-04-23T12:23:12Z

@swift-ci test

…xd/test-toolchain-argument # Conflicts: # Sources/PackageModel/UserToolchain.swift

MaxDesiatov · 2024-05-02T19:27:10Z

@swift-ci test

xedin · 2024-05-16T21:26:23Z

For posterity: I changed the logic here slightly, --toolchain gets prepended (to override existing entries) but otherwise we keep the ordering of the entries in rootPaths the same which means that we are going to select a tool from the first matching root in order they were provided and the fact that we weren't doing that before seems unintentional.

xedin · 2024-05-17T01:09:03Z

@swift-ci please clean test Linux platform

xedin · 2024-05-17T06:05:18Z

@swift-ci please clean test Linux platform

xedin · 2024-05-17T07:25:38Z

@swift-ci please clean test Linux platform

This reverts commit 133fbee.

Drop `SWIFTPM_CUSTOM_BIN_DIR` and related files because they are not passed through to the `hostSwiftSDK` anyway and even if they were we cannot use non-existant binaries because `targetTriple` of host toolchain gets computed by invoking `swiftc -print-target-info`.

xedin · 2024-05-17T17:45:06Z

@swift-ci please clean test Linux platform

xedin · 2024-05-18T03:52:34Z

For posterity there are currently multiple issues here

_hostToolchain doesn't use environment and fileSystem provided instead if fallback to .process() and localFileSystem which cannot accommodate overrides that are only available in-memory
Changing only environment runs into issues with SWIFTPM_CUSTOM_BIN_DIR
Changing both environment and fileSystem results in a few problems on Linux:
- CI specifies SWIFT_EXEC* which wouldn't be available in in-memory filesystem
- Dropping that crashes in UserToolchain.init when it tries to determine targetTriple for host SDK by running /swiftpm/bin/swiftc -print-target-info

MaxDesiatov · 2024-05-18T12:17:44Z

Dropping that crashes in UserToolchain.init when it tries to determine targetTriple for host SDK by running /swiftpm/bin/swiftc -print-target-info

I worked on this one specifically in ce70582, 1f2a1ca and a few other recent commits. When specifying a fake environment with in-memory FS one should provide a predetermined host triple to avoid trying to spawn that process at a fake path.

xedin · 2024-05-18T15:54:08Z

@MaxDesiatov This cannot be done at the moment for SwiftCommandState which is under test, I should have mentioned explicitly that the only failing test on Linux is the new one in SwiftCommandStateTests.

We need to figure out whether we want to mock a host toolchain there, provide a host triple somehow or do something else…

…xd/test-toolchain-argument # Conflicts: # Tests/BuildTests/BuildPlanTests.swift

MaxDesiatov · 2024-06-03T16:11:58Z

@swift-ci test

…xd/test-toolchain-argument

MaxDesiatov · 2024-06-05T16:47:21Z

@swift-ci test

MaxDesiatov · 2024-06-05T17:19:10Z

@swift-ci test linux

MaxDesiatov · 2024-06-05T17:31:04Z

@swift-ci test macos

MaxDesiatov · 2024-06-05T17:46:02Z

@swift-ci test windows

MaxDesiatov · 2024-06-05T19:03:37Z

@swift-ci test windows

xedin · 2024-06-05T20:47:01Z

Sources/CoreCommands/SwiftCommandState.swift

+                environment: self.environment,
+                observabilityScope: self.observabilityScope
+            )
+            hostSwiftSDK.targetTriple = self.hostTriple


Not for this PR - maybe we should rename this to triple?

On SwiftSDK or on SwiftCommandState?

SwiftSDKs have both triples specified on purpose, as one can have a Swift SDK installed supporting certain host triples that don't include the triple of a machine it's installed on. hostTriple is optional on it for Swift SDKs that don't have such requirements. But we still need to distinguish target from host there.

Behavior of `--toolchain` regressed in #7296 by preferring the first match in toolset root paths: when `swiftc` is available in the same directory next to the SwiftPM binary being invoked, that became preferred with `--toolchain` value ignored. This is now fixed by slightly tweaking where custom toolchain gets added - it's prepended. A newly added test verifies that the behavior is fixed. For that we had to inject `FileSystem` and `EnvironmentVariables` in a lot more places, with an added benefit of making our tests more consistent and isolated from an arbitrary build environment. Resolves: rdar://126095653 --------- Co-authored-by: Pavel Yaskevich <[email protected]> (cherry picked from commit ee9d0b2) # Conflicts: # Sources/PackageModel/UserToolchain.swift

@bnbarham

Cherry-pick of #7483. **Explanation**: Behavior of `--toolchain` regressed in https://github.com/apple/swift-package-manager/pull/7296/files by preferring the first match in toolset root paths: when `swiftc` is available in the same directory next to the SwiftPM binary being invoked, that became preferred with `--toolchain` value ignored. **Scope**: limited to cross-compilation and infrequent builds that utilize this option. **Risk**: low due to limited scope, merged to `main` for a week with no regressions discovered. **Testing**: A newly added test verifies that the behavior is fixed. For that we had to inject `FileSystem` and `EnvironmentVariables` in a lot more places, with an added benefit of making our tests more consistent and isolated from an arbitrary build environment. **Issue**: rdar://126095653 **Reviewer**: @bnbarham

Cover –-toolchain argument in SwiftCommandStateTests

d91abb3

We should prevent possible regressions in how this argument is handled.

MaxDesiatov added cross-compilation test suite improvements to SwiftPM test suite build system Changes to interactions with build systems swift build Changes impacting `swift build` labels Apr 23, 2024

MaxDesiatov self-assigned this Apr 23, 2024

MaxDesiatov requested review from bnbarham and tomerd as code owners April 23, 2024 11:35

MaxDesiatov added a commit that referenced this pull request Apr 23, 2024

Remove TSCBasic.InMemoryFileSystem dependency

314fb0a

Since #7483 this type is implemented in SwiftPM.

MaxDesiatov mentioned this pull request Apr 23, 2024

[NFC] Remove TSCBasic.InMemoryFileSystem dependency #7484

Merged

Fix InMemoryFileSystem.swift not present in CMakeLists.txt

82c0b88

MaxDesiatov mentioned this pull request Apr 23, 2024

[6.0] Fix –-toolchain value shadowed by local executables #7485

Merged

Fix SourceKitLSPAPITests build error

e5530ce

Fix testAndroidCompilerFlags

e546eb7

Formatting cleanup

dc40bcb

MaxDesiatov marked this pull request as draft April 23, 2024 16:20

MaxDesiatov added 5 commits April 23, 2024 17:53

Reproduce the issue with SWIFTPM_CUSTOM_BIN_DIR

a16b586

Merge branch 'main' of github.com:apple/swift-package-manager into ma…

3579dcd

…xd/test-toolchain-argument # Conflicts: # Sources/PackageModel/UserToolchain.swift

Prepend toolset paths in SwiftSDK to respect --toolchain option

ff1ed06

Restore environment after updating it

f903c7f

Inject environment consistent with injected file system

5b47636

MaxDesiatov changed the title ~~Cover –-toolchain argument in SwiftCommandStateTests~~ Fix –-toolchain behavior regression May 2, 2024

MaxDesiatov changed the title ~~Fix –-toolchain behavior regression~~ Fix –-toolchain value shadowed by local executables May 2, 2024

MaxDesiatov marked this pull request as ready for review May 2, 2024 19:19

Fix Workspace environment injection

f8f7822

bnbarham approved these changes May 16, 2024

View reviewed changes

xedin force-pushed the maxd/test-toolchain-argument branch from 3ecfea4 to a68d7cf Compare May 17, 2024 07:25

xedin added 2 commits May 17, 2024 10:13

Revert "[CoreCommands] Pipe environment through to the host toolchain"

dfa56d3

This reverts commit 133fbee.

xedin force-pushed the maxd/test-toolchain-argument branch from a68d7cf to a7f8121 Compare May 17, 2024 17:44

Merge branch 'main' of github.com:apple/swift-package-manager into ma…

ad05fcb

…xd/test-toolchain-argument # Conflicts: # Tests/BuildTests/BuildPlanTests.swift

MaxDesiatov force-pushed the maxd/test-toolchain-argument branch from 72a9833 to ad05fcb Compare June 4, 2024 16:32

MaxDesiatov added 2 commits June 5, 2024 15:09

Merge branch 'main' of github.com:apple/swift-package-manager into ma…

1674834

…xd/test-toolchain-argument

Fix testToolchainArgument on all platforms

3ed79e8

MaxDesiatov requested a review from xedin June 5, 2024 16:47

Override host triple in SwiftCommandStateTests

e4968d0

xedin reviewed Jun 5, 2024

View reviewed changes

MaxDesiatov merged commit ee9d0b2 into main Jun 6, 2024

MaxDesiatov deleted the maxd/test-toolchain-argument branch June 6, 2024 09:43

Fix –-toolchain value shadowed by local executables #7483

Fix –-toolchain value shadowed by local executables #7483

Uh oh!

Conversation

MaxDesiatov commented Apr 23, 2024 • edited by xedin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov commented Apr 23, 2024

Uh oh!

MaxDesiatov commented Apr 23, 2024

Uh oh!

MaxDesiatov commented Apr 23, 2024

Uh oh!

MaxDesiatov commented Apr 23, 2024

Uh oh!

MaxDesiatov commented May 2, 2024

Uh oh!

xedin commented May 16, 2024

Uh oh!

xedin commented May 17, 2024

Uh oh!

xedin commented May 17, 2024

Uh oh!

xedin commented May 17, 2024

Uh oh!

xedin commented May 17, 2024

Uh oh!

xedin commented May 18, 2024

Uh oh!

MaxDesiatov commented May 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented May 18, 2024

Uh oh!

MaxDesiatov commented Jun 3, 2024

Uh oh!

MaxDesiatov commented Jun 5, 2024

Uh oh!

MaxDesiatov commented Jun 5, 2024

Uh oh!

MaxDesiatov commented Jun 5, 2024

Uh oh!

MaxDesiatov commented Jun 5, 2024

Uh oh!

MaxDesiatov commented Jun 5, 2024

Uh oh!

xedin Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

xedin Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix `–-toolchain` value shadowed by local executables #7483

Fix `–-toolchain` value shadowed by local executables #7483

MaxDesiatov commented Apr 23, 2024 •

edited by xedin

Loading

MaxDesiatov commented May 18, 2024 •

edited

Loading

MaxDesiatov Jun 6, 2024 •

edited

Loading